http: moving functions and member variables out of http1 pool#13867
http: moving functions and member variables out of http1 pool#13867ggreenway merged 4 commits intoenvoyproxy:masterfrom
Conversation
…ool. Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
force-pushed
the
http_changes
branch
from
903e272 to
3ae4157
Compare
|
@ggreenway with Snow out can you do first pass? |
| Upstream::Host::CreateConnectionData data = parent_.host()->createConnection( | ||
| parent_.dispatcher(), parent_.socketOptions(), parent_.transportSocketOptions()); | ||
| Upstream::Host::CreateConnectionData data = | ||
| static_cast<Envoy::ConnectionPool::ConnPoolImplBase*>(&parent)->host()->createConnection( |
There was a problem hiding this comment.
What's going on with this cast? Isn't this an upcast, and thus would happen implicitly?
There was a problem hiding this comment.
Annoyingly,
HttpConnPoolImplBase has
Upstream::HostDescriptionConstSharedPtr host() const override { return host_; }
and the base class has
const Upstream::HostConstSharedPtr& host() const { return host_; }
so without the cast it snags the wrong type and complains there's no createConnection function.
There was a problem hiding this comment.
Yuck. Can you add a comment to that effect?
There was a problem hiding this comment.
yeah - will add it to my clean up list too - may be able to do away with it with one of the later refactors!
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
Thanks for the review!
| Upstream::Host::CreateConnectionData data = parent_.host()->createConnection( | ||
| parent_.dispatcher(), parent_.socketOptions(), parent_.transportSocketOptions()); | ||
| Upstream::Host::CreateConnectionData data = | ||
| static_cast<Envoy::ConnectionPool::ConnPoolImplBase*>(&parent)->host()->createConnection( |
There was a problem hiding this comment.
Annoyingly,
HttpConnPoolImplBase has
Upstream::HostDescriptionConstSharedPtr host() const override { return host_; }
and the base class has
const Upstream::HostConstSharedPtr& host() const { return host_; }
so without the cast it snags the wrong type and complains there's no createConnection function.
As part of #3431 we need to move the logic from HTTP/1 and HTTP/2 connection pools to the active client, so the new mixed connection pool can just create an active client of the right type and have all the logic in the client, not split between the client and the pool. This removes functions for the HTTP/2 pool not moved in #13867 Risk Level: Medium (data plane refactor) Testing: existing tests pass Docs Changes: n/a Release Notes: n/a Part of #3431 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
3 participants
As part of #3431 we need to move the logic from HTTP/1 and HTTP/2 connection pools to the active client,
so the new mixed connection pool can just create an active client of the right type.
This removes member variables from the HTTP/1.1 connection pool in preparation for that move.
a follow-up PR will do the same for HTTP/2, then the two classes will be removed in favor of the base http connection
pool being created with an instantiate_active_client_ closure which creates the right type of clients.
The alpn pool will eventually create an active client of the right type based on initial ALPN.
Risk Level: High (data plane refactor, albeit intended no-op)
Testing: existing tests pass
Docs Changes: n/a
Release Notes: n/a